-
Notifications
You must be signed in to change notification settings - Fork 453
feat: split AllocationManager
#1643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: split AllocationManager
#1643
Conversation
e64c0f7 to
7dc8bb1
Compare
AllocationManagerAllocationManager
bec851a to
7dc8bb1
Compare
7dc8bb1 to
b480b22
Compare
bce9cbb to
c572ec5
Compare
99855a2 to
c676cd8
Compare
42af1f1 to
ecc69cf
Compare
src/test/integration/tests/upgrade/AllocationManagerUpgrade.t.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get some tests here to ensure that only view functions can be delegate called? And revert otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of an odd thing to test, since the compiler explicitly does this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for this would actually fail. If the delegate implements a non-view function, it can be called with delegateView successfully. _delegateView does not enforce that delegate calls do not modify state; it's more of a suggestion that the underlying function doesn't modify state.
If you're curious, try plugging the below code block into SplitContractMixin.t.sol locally, and run forge t SplitContractMixin.t.sol. You'll see that test_setValue passes, indicating that setValue (which is a view function and uses delegateView) is able to execute a state-modifying operation.
// rest of SplitContractMixinTest above
function setValue(uint) public view returns (uint result) {
_delegateView(address(delegate));
result;
}
function test_setValue() public {
(bool success, bytes memory data) = address(this).call(abi.encodeWithSelector(this.setValue.selector, 100));
assertTrue(success);
(bool success, bytes memory data) = address(this).call(abi.encodeWithSelector(this.getValue.selector));
assertTrue(success);
uint result = abi.decode(data, (uint));
assertEq(result, 100);
}
}
// Mock contract to test delegation
contract Delegate is Test {
uint value;
function getValue() public view returns (uint result) {
return value;
}
function setValue(uint _value) public {
value = _value;
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nadir-akhtar We're talking about different things here, @0xrajath was suggesting adding tests to confirm that if we send calldata with a selector that's unknown to the view contract it reverts. I'm saying it's standard solidity behavior for a contract to revert if the functions unrecognized.
src/test/integration/tests/upgrade/AllocationManagerUpgrade.t.sol
Outdated
Show resolved
Hide resolved
nadir-akhtar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split looks like a solid start! Especially with the layout at <storage slot> syntax making this super convenient.
A few notes on lingering TODOs, as well as some room for clarification in our docs on _delegateView allowing for delegatecall effects that may change state if the delegate contract implements such a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for this would actually fail. If the delegate implements a non-view function, it can be called with delegateView successfully. _delegateView does not enforce that delegate calls do not modify state; it's more of a suggestion that the underlying function doesn't modify state.
If you're curious, try plugging the below code block into SplitContractMixin.t.sol locally, and run forge t SplitContractMixin.t.sol. You'll see that test_setValue passes, indicating that setValue (which is a view function and uses delegateView) is able to execute a state-modifying operation.
// rest of SplitContractMixinTest above
function setValue(uint) public view returns (uint result) {
_delegateView(address(delegate));
result;
}
function test_setValue() public {
(bool success, bytes memory data) = address(this).call(abi.encodeWithSelector(this.setValue.selector, 100));
assertTrue(success);
(bool success, bytes memory data) = address(this).call(abi.encodeWithSelector(this.getValue.selector));
assertTrue(success);
uint result = abi.decode(data, (uint));
assertEq(result, 100);
}
}
// Mock contract to test delegation
contract Delegate is Test {
uint value;
function getValue() public view returns (uint result) {
return value;
}
function setValue(uint _value) public {
value = _value;
}
}
nadir-akhtar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Much needed improvement to our contract codesize 💪
ypatil12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left one non-blocking comment
| check_IncrAlloc_State_Slashable(operator, allocateParams); | ||
|
|
||
| // 7. Roll blocks to complete allocation | ||
| _rollBlocksForCompleteAllocation(operator, operatorSet, strategies); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also slash here just for completeness to write to every (I think?) slot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in semver removal pr for sake of avoiding collecting approvals again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
82a9ddd
into
release-dev/slashing-ux-improvements
**Motivation:** The `AllocationManager` contract was hitting the 24KB bytecode size limit, which would have blocked deployment. We needed a solution to reduce the contract size while maintaining backwards compatibility with existing integrations that call view functions on `AllocationManager`. **Modifications:** - Created a new `SplitContractMixin` that uses a fallback to delegate unmatched function calls via `delegatecall` to a secondary contract - Split `AllocationManager` into two contracts: the main contract handles state-mutating operations, while `AllocationManagerView` handles all read-only view functions - Both contracts inherit from `AllocationManagerStorage` to share the same storage layout, enabling the view contract to read from the main contract's storage - Updated `AllocationManager` constructor to accept and store the `AllocationManagerView` address - Modified all deployment scripts (devnet, local, integration) to deploy both proxies and implementations - Updated `ExistingDeploymentParser`, test harnesses, and unit/integration tests to work with the split architecture **Result:** - The `AllocationManager` is now about ~ 4.8KB under the 24KB limit with room to grow. - The `AllocationManagerView` contract has 16KB of available space for future view functions. TODO: Add a ci check (I don't have privs but the code is written locally) 1) Get list of all contracts ending in View.sol in the repo. 2) Use forge inspect abi and check all mutability fields == view|pure. 3) Basic search over the file to see if sstore or delegatecall is used on a for additional sanity. --------- Co-authored-by: eigenmikem <[email protected]> squash squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci docs todo cleanup ci ci rename `secondHalf` rm extsload move storage add protocol registry to zeus squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci wip wip wip wip wip remove extsload wip tests passing wip passing perf: wrap modifier logic to save codesize chore: forge fmt refactor: remove semver (#1641) **Motivation:** We want to consolidate semver logic into a single purpose built contract. **Modifications:** - Removed `SemVerMixin` import from all contracts. **Result:** More codesize savings. make fmt refactor: expand `ProtocolRegistry` to list all contracts + pause rm gas snapshots rm extsload rebase ci revert eigen changes remove stale scripts
**Motivation:** The `AllocationManager` contract was hitting the 24KB bytecode size limit, which would have blocked deployment. We needed a solution to reduce the contract size while maintaining backwards compatibility with existing integrations that call view functions on `AllocationManager`. **Modifications:** - Created a new `SplitContractMixin` that uses a fallback to delegate unmatched function calls via `delegatecall` to a secondary contract - Split `AllocationManager` into two contracts: the main contract handles state-mutating operations, while `AllocationManagerView` handles all read-only view functions - Both contracts inherit from `AllocationManagerStorage` to share the same storage layout, enabling the view contract to read from the main contract's storage - Updated `AllocationManager` constructor to accept and store the `AllocationManagerView` address - Modified all deployment scripts (devnet, local, integration) to deploy both proxies and implementations - Updated `ExistingDeploymentParser`, test harnesses, and unit/integration tests to work with the split architecture **Result:** - The `AllocationManager` is now about ~ 4.8KB under the 24KB limit with room to grow. - The `AllocationManagerView` contract has 16KB of available space for future view functions. TODO: Add a ci check (I don't have privs but the code is written locally) 1) Get list of all contracts ending in View.sol in the repo. 2) Use forge inspect abi and check all mutability fields == view|pure. 3) Basic search over the file to see if sstore or delegatecall is used on a for additional sanity. --------- Co-authored-by: eigenmikem <[email protected]> squash squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci docs todo cleanup ci ci rename `secondHalf` rm extsload move storage add protocol registry to zeus squash feat: split alm wip wip wip wip wip wip wip unit tests passing remove extsload wip tests passing chore: git mv storage to folder wip ci wip wip wip wip wip remove extsload wip tests passing wip passing perf: wrap modifier logic to save codesize chore: forge fmt refactor: remove semver (#1641) **Motivation:** We want to consolidate semver logic into a single purpose built contract. **Modifications:** - Removed `SemVerMixin` import from all contracts. **Result:** More codesize savings. make fmt refactor: expand `ProtocolRegistry` to list all contracts + pause rm gas snapshots rm extsload rebase ci revert eigen changes remove stale scripts
**Motivation:** The `AllocationManager` contract was hitting the 24KB bytecode size limit, which would have blocked deployment. We needed a solution to reduce the contract size while maintaining backwards compatibility with existing integrations that call view functions on `AllocationManager`. **Modifications:** - Created a new `SplitContractMixin` that uses a fallback to delegate unmatched function calls via `delegatecall` to a secondary contract - Split `AllocationManager` into two contracts: the main contract handles state-mutating operations, while `AllocationManagerView` handles all read-only view functions - Both contracts inherit from `AllocationManagerStorage` to share the same storage layout, enabling the view contract to read from the main contract's storage - Updated `AllocationManager` constructor to accept and store the `AllocationManagerView` address - Modified all deployment scripts (devnet, local, integration) to deploy both proxies and implementations - Updated `ExistingDeploymentParser`, test harnesses, and unit/integration tests to work with the split architecture **Result:** - The `AllocationManager` is now about ~ 4.8KB under the 24KB limit with room to grow. - The `AllocationManagerView` contract has 16KB of available space for future view functions. TODO: Add a ci check (I don't have privs but the code is written locally) 1) Get list of all contracts ending in View.sol in the repo. 2) Use forge inspect abi and check all mutability fields == view|pure. 3) Basic search over the file to see if sstore or delegatecall is used on a for additional sanity. --------- Co-authored-by: eigenmikem <[email protected]>
Motivation:
The
AllocationManagercontract was hitting the 24KB bytecode size limit, which would have blocked deployment. We needed a solution to reduce the contract size while maintaining backwards compatibility with existing integrations that call view functions onAllocationManager.Modifications:
SplitContractMixinthat uses a fallback to delegate unmatched function calls viadelegatecallto a secondary contractAllocationManagerinto two contracts: the main contract handles state-mutating operations, whileAllocationManagerViewhandles all read-only view functionsAllocationManagerStorageto share the same storage layout, enabling the view contract to read from the main contract's storageAllocationManagerconstructor to accept and store theAllocationManagerViewaddressExistingDeploymentParser, test harnesses, and unit/integration tests to work with the split architectureResult:
AllocationManageris now about ~ 4.8KB under the 24KB limit with room to grow.AllocationManagerViewcontract has 16KB of available space for future view functions.TODO: Add a ci check (I don't have privs but the code is written locally)